-
Notifications
You must be signed in to change notification settings - Fork 801
deps: Default gtk4-layer-shell system integration to true #6706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Dissatified by:
|
Open issues in Zig related to RPath handling in the build system ziglang/zig#17373 |
FWIW, I applied this on top of 73c7943 on origin/main and can confirm that after removing
I don't feel qualified to comment on any other aspects of it, but for now I'm going to carry the patch around to survive reboots. |
5f712d5
to
9348e35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense pending the linker path comment
Just a heads up/FYI, but the patch as-is at time of writing no longer cleanly applies after cfea2ea; cf. this bit (the diff of I have the library in |
@jwbowen Couldn't you stick to an older commit (i.e.: not upgrade for a bit) until this PR is merged? |
I certainly could, but I really like riding the bleeding edge where I can and am willing to go out of my way to make it work. I'm not concerned with practicality in this case :) I should have been clearer in my last message that I wasn't complaining about the patch, I just wanted to give a heads up/FYI. Edit: I updated my previous comment to hopefully make that clearer. I'm very excited about the project :) |
382abeb
to
8626fda
Compare
8626fda
to
4e21991
Compare
Should I leave the install step config at https://github.com/ghostty-org/ghostty/pull/6706/files#diff-3c27f71e7b0ef00072a7d994ae2af66c62b15fc7301fb43aad52bcd96a64da24R653-R654 or is there a better place to organise it? At this point, an absolute rpath link is made in Debug mode but we default to system integration for
Otherwise I think this is in a working state where docs for building on Debian 12 (missing gtk4-layer-shell) need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for plugging away at this. A couple points of feedback.
I think this is looking fine. I still want to consider the release packaging implications. I think this will fix debug builds just fine but release builds seem like there's going to be issues and I wonder if we need to reintroduce a way to wholesale disable gtk4-layer-shell from some builds... (don't bother in this PR)
4e21991
to
ca79121
Compare
ca79121
to
e53a023
Compare
e53a023
to
b3829cb
Compare
Checking in on this, adding the RPath config for an installed library only in debug mode for development seems to be no improvement over relying on Zig's default behaviour of placing the RPath of the build cache's version of the library. I think Zig defaults this RPath config to cache objects to make build script executables work, though forgot the source. |
We default system-integration to true as this is a shared library that must be installed on a library path and it is recommended to use the system package. If the system does not package gtk4-layer-shell then doing `zig build -fno-sys` will now correctly build and install the shared library under a lib/ subdirectory of the output prefix. The output prefix should be a default library path (`/lib`, `/usr/lib`, or a lib64 variant) otherwise a custom library path can be configured using ldconfig (see `man ld.so 8`)
b3829cb
to
da0bb6a
Compare
Defaulting this to true should have implications on our snap build, flatpak build and nix develop to have gtk4-layer-shell system installed |
13449d4
to
8996398
Compare
Closes #6632
When compiling the dynamic lib and linking, the rpath resolves to the compile cache location instead of the install location for the lib. This resulted in loading the dylib failing when the compile cache was removed or the install location is changed.
Based on ziglang/zig#5827 , we specify the rpath to search relative to the executable.
Previous state:
$OUT/lib/libgtk4-layer-shell.so
Before:
After: